rs/moq/src/coding/varint.rs (1)

206-214: Minor: prefer bit-shifts over integer pow for clarity and zero-cost constants

Shifts read more directly as bit-widths and avoid calling pow at runtime (however cheap). Purely a readability/micro-optimization nit.

Apply this diff:

-    if self.0 < 2u64.pow(6) {
+    if self.0 < (1u64 << 6) {
       w.put_u8(self.0 as u8);
-    } else if self.0 < 2u64.pow(14) {
+    } else if self.0 < (1u64 << 14) {
       w.put_u16((0b01 << 14) | self.0 as u16);
-    } else if self.0 < 2u64.pow(30) {
+    } else if self.0 < (1u64 << 30) {
       w.put_u32((0b10 << 30) | self.0 as u32);
     } else {
       w.put_u64((0b11 << 62) | self.0);
     }

js/moq/package.json (1)

23-25: Consider making the WS polyfill optional to keep installs lean.

If the polyfill is only dynamically imported on fallback, publishing it as an optional dependency avoids pulling it into environments that never use it.

   "dependencies": {
-    "async-mutex": "^0.5.0",
-    "@kixelated/web-transport-ws": "^0.1"
+    "async-mutex": "^0.5.0"
   },
+  "optionalDependencies": {
+    "@kixelated/web-transport-ws": "^0.1"
+  },

If you do need it always present (e.g., for bundlers without dynamic externals), keep it in dependencies.

rs/moq-relay/cfg/root.toml (2)

15-19: Port selection: TCP 4443 for HTTP may be confusing alongside QUIC on UDP 4443.

Sharing the same numeric port across TCP/UDP is valid, but 4443 usually implies TLS. Consider a distinct dev port (e.g., 4444) or add a clarifying comment to avoid operator confusion.

-[web.http]
-# Listen for HTTP and WebSocket (TCP) connections on the given address.
-# Defaults to disabled if not provided.
-listen = "[::]:4443"
+[web.http]
+# Listen for HTTP and WebSocket (TCP) connections on the given address.
+# Defaults to disabled if not provided.
+# Note: QUIC uses UDP:4443 above; choose a distinct TCP port for clarity in dev.
+listen = "[::]:4444"

---

15-19: Call out security/mixed‑content expectations for WS.

When serving from https origins, ws:// will be blocked as mixed content; operators will need wss://. A brief note here will prevent surprises.

 [web.http]
 # Listen for HTTP and WebSocket (TCP) connections on the given address.
 # Defaults to disabled if not provided.
 listen = "[::]:4443"
+#
+# Tip: Browsers block ws:// from https pages. Use a TLS terminator/reverse proxy
+# to expose wss:// in production even if QUIC/TLS is optional.

js/tsconfig.json (1)

9-11: Split debug maps into a dev tsconfig instead of commenting out.

Source maps and declaration maps are useful during local debugging but can stay off for publish. Consider a tsconfig.dev.json that extends this and enables both.

Example:

• tsconfig.dev.json
  • { "extends": "./tsconfig.json", "compilerOptions": { "sourceMap": true, "declarationMap": true } }
• Use it in dev scripts only.

js/moq/src/stream.ts (2)

69-81: Empty chunk handling may be too strict; also avoid an extra copy.

Throwing on zero-length chunks could break defensive polyfills; skipping them is safer. Also, wrapping result.value in a new Uint8Array copies data unnecessarily.

-    const result = await this.#reader.read();
-    if (result.done) {
-      return false;
-    }
-
-    if (result.value.byteLength === 0) {
-      throw new Error("unexpected empty chunk");
-    }
-
-    const buffer = new Uint8Array(result.value);
+    const result = await this.#reader.read();
+    if (result.done) return false;
+    const chunk = result.value;
+    // Some environments may yield empty chunks; ignore them defensively.
+    if (chunk.byteLength === 0) return true;
+    const buffer = chunk;

---

187-193: Use getUint16 for clarity (unsigned) when decoding 2‑byte varints.

Semantics are unsigned; masking fixes it, but using Uint avoids confusion.

-      const view = new DataView(slice.buffer, slice.byteOffset, slice.byteLength);
-      return BigInt(view.getInt16(0)) & 0x3fffn;
+      const view = new DataView(slice.buffer, slice.byteOffset, slice.byteLength);
+      return BigInt(view.getUint16(0)) & 0x3fffn;

js/justfile (1)

11-11: Use frozen installs in CI and make it the default via flags.

Reproducible installs reduce heisenbugs in CI. You already accept flags—default them to --frozen-lockfile.

-web url='http://localhost:4443/anon':
-	pnpm i
+web url='http://localhost:4443/anon':
+	pnpm install
@@
-check flags="":
-	pnpm install {{flags}}
+check flags="--frozen-lockfile":
+	pnpm install {{flags}}
@@
-fix flags="":
+fix flags="--frozen-lockfile":
 	# Fix the JS packages
 	pnpm install {{flags}}
@@
-build flags="":
-	pnpm install {{flags}}
+build flags="--frozen-lockfile":
+	pnpm install {{flags}}

Also applies to: 16-16, 33-33, 47-47

rs/moq-relay/cfg/leaf.toml (1)

15-18: Comment/port mismatch: QUIC comment says 4443 but listen/advertise use 4444

The new [web.http] TCP listener on 4444 looks fine alongside QUIC on UDP 4444, but the comment above still claims UDP:4443, which is misleading.

Apply one of:

-# Listen for QUIC connections on UDP:4443
+# Listen for QUIC connections on UDP:4444

or, if the intent was to keep QUIC on 4443:

-listen = "[::]:4444"
+listen = "[::]:4443"

Also applies to: 7-9

rs/moq-relay/src/auth.rs (2)

22-32: Return more precise HTTP status codes per auth failure

All AuthError variants currently map to 401. Suggest finer mapping to aid clients and logging:

• UnexpectedToken -> 400 Bad Request
• ExpectedToken, DecodeFailed -> 401 Unauthorized
• IncorrectRoot -> 403 Forbidden

-impl From<AuthError> for http::StatusCode {
-    fn from(_: AuthError) -> Self {
-        http::StatusCode::UNAUTHORIZED
-    }
-}
+impl From<AuthError> for http::StatusCode {
+    fn from(err: AuthError) -> Self {
+        match err {
+            AuthError::UnexpectedToken => http::StatusCode::BAD_REQUEST,
+            AuthError::ExpectedToken | AuthError::DecodeFailed => http::StatusCode::UNAUTHORIZED,
+            AuthError::IncorrectRoot => http::StatusCode::FORBIDDEN,
+        }
+    }
+}
 
 impl axum::response::IntoResponse for AuthError {
-    fn into_response(self) -> axum::response::Response {
-        http::StatusCode::UNAUTHORIZED.into_response()
-    }
+    fn into_response(self) -> axum::response::Response {
+        let status: http::StatusCode = self.clone().into();
+        // Optional: include a short error string to aid debugging without leaking sensitive details.
+        (status, self.to_string()).into_response()
+    }
 }

---

95-110: Token/key handling edge: “UnexpectedToken” semantics

Returning UnexpectedToken when a token is provided but no key is configured is sensible; just confirm the web layer doesn’t prompt clients to retry with a token in this case. If you adopt the status code change above, this will become a 400 which matches the situation.

rs/moq-relay/Cargo.toml (1)

17-18: WS feature enablement and deps LGTM; consider minimal-feature axum.

The ws feature on axum and web-transport-ws dep fit the WS fallback. Optionally disable axum default features to keep the binary lean.

Suggested tweak (optional):

-axum = { version = "0.8", features = ["tokio", "ws"] }
+axum = { version = "0.8", default-features = false, features = ["tokio", "ws"] }

Also applies to: 20-20, 36-36

js/moq/src/lite/connection.ts (1)

115-124: Always-log finally is good; consider adding a local catch for decode errors for better context.

Right now any decode error surfaces only as a generic "fatal error running connection" from #run(). A localized error makes triage easier without changing behavior.

Apply:

 async #runSession() {
-    try {
+    try {
       // Receive messages until the connection is closed.
       for (;;) {
         const msg = await SessionInfo.decodeMaybe(this.#session.reader);
         if (!msg) break;
         // TODO use the session info
       }
+    } catch (err) {
+      if (!this.#closed) {
+        console.error("error reading session info", err);
+      }
     } finally {
       console.warn("session stream closed");
     }
   }

rs/moq-relay/cfg/dev.toml (1)

18-23: Bind HTTP to loopback by default in dev to avoid accidental exposure

Listening on all interfaces in dev can leak the fingerprint/track endpoints on shared machines. Prefer loopback.

Apply:

 [web.http]
 # Listen for HTTP and WebSocket connections on the given TCP address.
 # This is unfortunately required to serve certificate.sha256 for local development.
 # However, as a bonus, we can serve tracks via both HTTP and WebSocket fallbacks.
-listen = "[::]:4443"
+listen = "127.0.0.1:4443"

rs/moq-relay/cfg/prod.toml (3)

15-18: Document operational caveats for optional HTTP on :80

If enabled, binding :80 on Linux requires root or CAP_NET_BIND_SERVICE; behind a TLS terminator you likely don’t want a cleartext listener. Add a note to prevent misconfig.

---

19-27: Avoid TLS config duplication; ensure fail-fast if certs are missing

You repeat cert/key under both [server.tls] and [web.https]. Easy to drift. Consider a single tls.* block referenced by both, or at least add a comment stating they must be identical. Also ensure startup fails fast if either file is unreadable.

---

4-8: Same port (443) on UDP and TCP is OK; add a hint about firewall rules

Using UDP:443 for QUIC and TCP:443 for HTTPS is correct; add a brief note reminding operators to open both protocols in security groups/firewalls.

rs/moq-relay/src/main.rs (1)

28-36: Fail fast on web bind errors instead of panicking inside a task

If web.run() errors (e.g., EADDRINUSE), the panic happens in a spawned task and the process may keep serving QUIC, which is confusing. Prefer surfacing bind errors before entering the accept loop.

Example pattern (outside this hunk):

// Bind or validate listeners first so errors are synchronous.
let web = Web::new(state, config.web);
// If Web exposes a bind/prepare step, call it here and handle errors.
// web.prepare().context("failed to prepare web listeners")?;

tokio::spawn(async move {
    if let Err(err) = web.run().await {
        tracing::error!(%err, "web server exited");
        // Optionally: std::process::exit(1);
    }
});

Would you like me to propose a minimal prepare() API in rs/moq-relay/src/web.rs?

rs/moq-relay/src/config.rs (1)

33-37: Add inline config tests for CLI-over-file precedence and defaults

Given serde(flatten) + deny_unknown_fields, add small unit tests here to lock behavior.

You can append:

#[cfg(test)]
mod tests {
    use super::*;
    use clap::Parser;

    #[test]
    fn cli_overrides_file() {
        // Simulate: file sets web.http.listen=:80, CLI sets :8080
        let mut cfg: Config = toml::from_str(r#"
            [web.http]
            listen = "[::]:80"
        "#).unwrap();
        // Emulate CLI args
        cfg.update_from(["bin", "--web.http.listen", "[::]:8080"]);
        assert_eq!(cfg.web.http.as_ref().unwrap().listen.unwrap().to_string(), "[::]:8080");
    }

    #[test]
    fn default_web_is_disabled() {
        let cfg = Config::parse_from(["bin"]);
        assert!(cfg.web.http.is_none() && cfg.web.https.is_none());
    }
}

rs/moq-relay/src/cluster.rs (2)

71-74: Comment wording nit: “publish” vs “subscribe”.
This block is for subscribing; the comment says “publish our primary broadcasts.”

Apply:

• “If this is a cluster node, then only serve our primary broadcasts.”
• “Otherwise serve everything.”

---

69-95: Add inline tests for token scoping.
Cover: cluster=true vs false, valid/invalid roots, subscribe/publish filters.

I can add a minimal #[cfg(test)] block validating these branches.

rs/moq-relay/src/connection.rs (2)

15-23: Prefer Authorization header over URL query for JWTs

JWTs in query params are commonly logged by proxies and appear in referrers. Support Authorization: Bearer ... and fall back to query only if necessary.

---

38-49: Tidy duplicate “accepted” logs

You log “session accepted” twice (Line 33 and match arms). With the prior change, keep the detailed per-role logs here and drop the earlier one.

rs/moq-native/src/server.rs (2)

164-168: Surface accept errors instead of silently dropping

Currently, Errs from accept_session are ignored. Log them so operators can see handshake/ALPN issues.

-                Some(res) = self.accept.next() => {
-                    if let Ok(session) = res {
-                        return Some(session)
-                    }
-                }
+                Some(res) = self.accept.next() => {
+                    match res {
+                        Ok(session) => return Some(session),
+                        Err(e) => tracing::warn!(error = ?e, "failed to accept session"),
+                    }
+                }

---

228-229: Typo in comment

“cooresponding” → “corresponding”.

-    // Load a certificate and cooresponding key from a file
+    // Load a certificate and corresponding key from a file

rs/moq/src/session/reader.rs (2)

20-28: Remove commented-out accept() or gate with cfg(test)

Dead/commented code adds noise. Either delete or wrap a minimal test helper behind cfg(test).

-    /*
-    pub async fn accept<S: transport::Session>(session: &S) -> Result<Self, Error>
-    where
-        S::RecvStream: T,
-    {
-        let stream = session.accept_uni().await?;
-        Ok(Self::new(stream))
-    }
-    */

---

81-93: closed() semantics are correct; consider a unit test

Behavior (error on unexpected trailing bytes) is precise. Add a small inline test to lock this in.

rs/moq/src/error.rs (1)

88-89: MSRV safety: avoid io::Error::other (use new + Other).

If you keep the manual Clone fallback, Error::other requires newer Rust. Prefer:

- std::io::Error::other("Transport error (cloned)")
+ std::io::Error::new(std::io::ErrorKind::Other, "Transport error (cloned)")

rs/moq/src/session/writer.rs (1)

17-29: Remove stale commented code.

Dead/commented API adds noise. Drop or move behind a feature gate.

rs/moq/src/session/subscriber.rs (2)

56-59: Don’t swallow uni stream errors; log them.

Currently errors from run_uni_stream() are dropped. Emit a debug log to aid triage.

-			web_async::spawn(async move {
-				this.run_uni_stream(stream).await.ok();
-			});
+			web_async::spawn(async move {
+				if let Err(err) = this.run_uni_stream(stream).await {
+					tracing::debug!(%err, "uni stream error");
+				}
+			});

---

127-143: Broadcasts map is never populated; remove or insert on announce.

self.broadcasts is only removed from (Line 171) but never inserted, so the map is effectively unused. Either remove the field or insert entries when starting a broadcast.

 		// Run the broadcast in the background until all consumers are dropped.
 		self.origin
 			.as_mut()
 			.unwrap()
 			.publish_broadcast(path.clone(), broadcast.consumer);
 
-		web_async::spawn(self.clone().run_broadcast(path, broadcast.producer));
+		// Track for lookup/cleanup.
+		self.broadcasts.lock().insert(path.clone(), broadcast.producer.clone());
+
+		web_async::spawn(self.clone().run_broadcast(path, broadcast.producer));

rs/moq-relay/src/web.rs (1)

162-169: Typo: “Unfortuantely” → “Unfortunately”.

-		// Unfortuantely, we need to convert from Axum to Tungstenite.
+		// Unfortunately, we need to convert from Axum to Tungstenite.

rs/moq/src/session/mod.rs (6)

20-22: Make transport private to preserve invariants and allow future changes.

Exposing the transport directly couples external callers to the concrete transport API and makes it harder to evolve. Prefer a private field with an accessor.

Apply this diff:

-pub struct Session<S: web_transport_trait::Session> {
-    pub transport: S,
-}
+pub struct Session<S: web_transport_trait::Session> {
+    transport: S,
+}

And add a lightweight accessor inside this impl block:

pub fn transport(&self) -> &S {
    &self.transport
}

---

42-63: Handle Error::Cancel as a normal shutdown to avoid warning noise and confusing close codes.

run_session() returns Error::Cancel on graceful end-of-stream. The current match treats it as an error (warn, non-zero close code via err.to_code()), which is misleading.

Apply this diff to add a dedicated branch before the generic Err(err) arm:

 			match res {
-				Err(Error::Transport(_)) => {
+				Err(Error::Transport(_)) => {
 					tracing::info!("session terminated");
 					session.close(1, "");
-				}
+				}
+				Err(Error::Cancel) => {
+					tracing::info!("session canceled");
+					session.close(0, "");
+				}
 				Err(err) => {
 					tracing::warn!(%err, "session error");
 					session.close(err.to_code(), err.to_string().as_ref());
 				}

Also, confirm close() is idempotent across concurrent callers (user calling Session::close() vs. the background task). If idempotency is not guaranteed by the transport, consider guarding with an atomic once flag.

---

69-69: Return a more specific error than Cancel on init failure.

When the announce init oneshot fails, mapping to Error::Cancel hides the cause (e.g., early stream close vs. setup error). Prefer a dedicated handshake/init error.

Example:

• Introduce Error::Handshake(&'static str) or Error::InitFailed.
• Use it here to aid diagnostics and metrics.

I can wire this through error.rs if you want.

---

143-146: Consider treating end-of-session as success to simplify callers.

Right now a clean end-of-stream yields Error::Cancel. If you adopt the dedicated match arm above, you can keep this; alternatively, returning Ok(()) avoids propagating a pseudo-error.

Alternative diff if you prefer the latter:

-while let Some(_info) = stream.reader.decode_maybe::<message::SessionInfo>().await? {}
-Err(Error::Cancel)
+while let Some(_info) = stream.reader.decode_maybe::<message::SessionInfo>().await? {}
+Ok(())

---

149-156: Minor: document close/closed semantics and reason lifetimes.

close(self, err) consumes self and passes err.to_string().as_ref(); this borrows a temporary, which is fine if close doesn't retain it. Add a brief doc note to clarify that the reason is used synchronously and not retained.

---

1-16: Add inline unit tests under #[cfg(test)] per repo guidelines.

Given the new generic transport, add tests using a mock S to validate:

• Successful connect/accept negotiation.
• Version mismatch path.
• Error::Cancel handling and close code mapping.

I can scaffold a #[cfg(test)] module with a minimal mock implementing web_transport_trait::Session to exercise connect_setup/accept_setup.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

Learnt from: CR
PR: kixelated/moq#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-13T21:30:35.001Z
Learning: Applies to rs/Cargo.toml : Maintain the Rust workspace configuration in rs/Cargo.toml

rs/moq-relay/src/web.rs (6)

198-201: Good fix: optional Path for /announced prefix.

Accepting Path<Option<String>> and defaulting to "" addresses the extractor failure when no suffix is provided.

---

96-104: Fix /certificate.sha256 handler and avoid panic when no fingerprints are present (HTTPS optional).

• Using expect("missing certificate") contradicts “HTTPS optional”.
• get(fingerprint) passes a String where a handler is required.

Apply:

-		// Get the first certificate's fingerprint.
-		// TODO serve all of them so we can support multiple signature algorithms.
-		let fingerprint = self.state.fingerprints.first().expect("missing certificate").clone();
-
-		let app = Router::new()
-			.route("/certificate.sha256", get(fingerprint))
-			.route("/announced", get(serve_announced))
-			.route("/announced/{*prefix}", get(serve_announced))
-			.route("/fetch/{*path}", get(serve_fetch));
+		// Get the first certificate's fingerprint, if any.
+		// TODO serve all of them so we can support multiple signature algorithms.
+		let fingerprint = self.state.fingerprints.first().cloned();
+
+		let mut app = Router::new()
+			.route("/announced", get(serve_announced))
+			.route("/announced/{*prefix}", get(serve_announced))
+			.route("/fetch/{*path}", get(serve_fetch));
+
+		if let Some(fp) = fingerprint {
+			// Serve a constant body with the fingerprint.
+			app = app.route("/certificate.sha256", get(move || async move { fp.clone() }));
+		}

---

150-151: Return 401 on auth failure instead of propagating internal errors (WS).

Map verification errors to UNAUTHORIZED so clients get a clear 401.

-	let token = state.auth.verify(&path, params.jwt.as_deref())?;
+	let token = state
+		.auth
+		.verify(&path, params.jwt.as_deref())
+		.map_err(|_| StatusCode::UNAUTHORIZED)?;

---

203-204: Return 401 on auth failure instead of propagating internal errors (/announced).

Consistent with WS and /fetch.

-	let token = state.auth.verify(&prefix, params.jwt.as_deref())?;
+	let token = state
+		.auth
+		.verify(&prefix, params.jwt.as_deref())
+		.map_err(|_| StatusCode::UNAUTHORIZED)?;

---

236-237: Return 401 on auth failure instead of propagating internal errors (/fetch).

Keeps API semantics consistent.

-	let token = state.auth.verify(&broadcast, params.jwt.as_deref())?;
+	let token = state
+		.auth
+		.verify(&broadcast, params.jwt.as_deref())
+		.map_err(|_| StatusCode::UNAUTHORIZED)?;

---

227-233: Harden path parsing for /fetch to tolerate extra slashes and empty segments.

Current split can yield empty segments (e.g., “/a//b/”), producing an empty track name. Guard and return 400 for invalid paths.

-	// The path containts a broadcast/track
-	let mut path: Vec<&str> = path.split("/").collect();
-	let track = path.pop().unwrap().to_string();
+	// The path contains a broadcast/track; tolerate leading/trailing/multiple slashes.
+	let mut path: Vec<&str> = path.split('/').filter(|s| !s.is_empty()).collect();
+	let Some(track) = path.pop().map(|s| s.to_string()) else {
+		return Err(StatusCode::BAD_REQUEST.into());
+	};
 
 	// We need at least a broadcast and a track.
 	if path.is_empty() {
 		return Err(StatusCode::BAD_REQUEST.into());
 	}

rs/moq-native/src/client.rs (1)

188-192: Bug: matching url.scheme() against moq_lite::ALPN makes the moql branch unreachable.

Compare to the literal "moql" (as done in the ALPN match above) to avoid hitting unreachable!().

-    let session = match url.scheme() {
-        "https" => web_transport_quinn::Session::connect(connection, url).await?,
-        moq_lite::ALPN => web_transport_quinn::Session::raw(connection, url),
-        _ => unreachable!(),
-    };
+    let session = match url.scheme() {
+        "https" => web_transport_quinn::Session::connect(connection, url).await?,
+        "moql"  => web_transport_quinn::Session::raw(connection, url),
+        _ => unreachable!(),
+    };

#!/bin/bash
# Find any other places comparing a URL scheme to an ALPN constant
rg -nP 'url\.scheme\(\).*moq_lite::ALPN|match\s+url\.scheme\(\)[\s\S]*moq_lite::ALPN'

rs/moq-native/src/server.rs (3)

113-119: Prevent booting without any certificates

If neither tls-cert/ tls-key nor tls-generate yields certs, the server starts but handshakes will fail. Enforce at least one cert loaded/generated.

 	let fingerprints = serve.fingerprints();
+	anyhow::ensure!(
+		!fingerprints.is_empty(),
+		"no TLS certificates configured; provide --tls-cert/--tls-key or --tls-generate"
+	);

---

202-209: ALPN handling inconsistent with advertised list

Only web_transport_quinn::ALPN is accepted; if moq_lite is intended, add a match arm; otherwise remove it from ALPN (see above).

---

120-123: Stop advertising moq_lite ALPN unless it’s actually handled

ALPN includes moq_lite, but accept_session rejects anything except WebTransport, causing misleading handshakes.

-		tls.alpn_protocols = vec![
-			web_transport_quinn::ALPN.as_bytes().to_vec(),
-			moq_lite::ALPN.as_bytes().to_vec(),
-		];
+		tls.alpn_protocols = vec![
+			web_transport_quinn::ALPN.as_bytes().to_vec(),
+		];

rs/moq/src/error.rs (1)

8-9: Good: transport errors now Arc-based and Clone-safe.

Switching to Arc<dyn Error + Send + Sync> restores derive(Clone) without losing error context. Matches prior guidance.

rs/moq/src/session/writer.rs (1)

33-47: Addresses earlier partial-write concern via write_all.

Introducing write_all(Buf) and keeping write private prevents accidental partial writes. This resolves the prior issue.

js/moq/src/connection.ts (1)

129-141: Harden fingerprint fetch: check HTTP status, trim, and validate hex.

A non-200 (e.g., HTML error) or trailing newlines will currently flow into Hex.toBytes, causing opaque failures.

-		// Fetch the fingerprint from the server.
-		const fingerprint = await Promise.race([fetch(fingerprintUrl), cancel]);
-		if (!fingerprint) return undefined;
-
-		const fingerprintText = await Promise.race([fingerprint.text(), cancel]);
-		if (fingerprintText === undefined) return undefined;
+		// Fetch the fingerprint from the server.
+		const res = await Promise.race([fetch(fingerprintUrl), cancel]);
+		if (!res) return undefined;
+		if (!res.ok) {
+			console.warn(fingerprintUrl.toString(), `fingerprint fetch failed: ${res.status} ${res.statusText}`);
+			return undefined;
+		}
+		const fingerprintText = (await Promise.race([res.text(), cancel]))?.trim();
+		if (!fingerprintText) return undefined;
+		// Optional: basic hex sanity check (64 hex chars for sha-256)
+		if (!/^[\da-fA-F]{64}$/.test(fingerprintText)) {
+			console.warn(fingerprintUrl.toString(), "invalid sha-256 fingerprint format");
+			return undefined;
+		}

rs/moq-relay/src/web.rs (1)

105-109: Limit WS fallback route to GET to avoid 400s on non-WS methods.

any(serve_ws) can intercept non-WS requests to unknown paths and return 400 due to extractor failure. GET is sufficient for WS handshakes.

-			true => app.route("/{*path}", any(serve_ws)),
+			true => app.route("/{*path}", get(serve_ws)),

rs/moq-native/Cargo.toml (1)

16-16: Consider gating anyhow's backtrace feature.

Enabling anyhow/backtrace globally increases build size; make it opt-in via a crate feature.

-anyhow = { version = "1", features = ["backtrace"] }
+anyhow = "1"

Add to this Cargo.toml (outside the shown range):

[features]
default = []
backtrace = ["anyhow/backtrace"]

rs/moq-native/src/client.rs (3)

169-177: De-duplicate scheme→ALPN and scheme→session logic.

Derive both from a single parsed enum to prevent drift between matches.

Example (outside shown range):

enum Scheme { Https, Moql }
impl TryFrom<&str> for Scheme {
    type Error = anyhow::Error;
    fn try_from(s: &str) -> anyhow::Result<Self> {
        match s { "https" => Ok(Scheme::Https), "moql" => Ok(Scheme::Moql), _ => anyhow::bail!("unsupported") }
    }
}

Use it to set ALPN and choose Session::{connect,raw}.

---

113-118: Gate BBR with a feature or config knob.

Keep it off by default, but make it easy to enable for experiments.

-//transport.congestion_controller_factory(Arc::new(quinn::congestion::BbrConfig::default()));
+#[cfg(feature = "bbr")]
+transport.congestion_controller_factory(Arc::new(quinn::congestion::BbrConfig::default()));

Add to Cargo.toml:

[features]
bbr = []

---

258-264: Use constant‑time compare for fingerprint equality.

Minor hardening: avoid timing leakage on mismatch.

-        if fingerprint.as_ref() == self.fingerprint.as_slice() {
+        if ring::constant_time::verify_slices_are_equal(
+            fingerprint.as_ref(),
+            self.fingerprint.as_slice()
+        ).is_ok() {

rs/moq-native/src/server.rs (2)

38-47: Make tls-cert/tls-key accept comma-delimited lists; clarify pairing semantics

Support comma-delimited env/CLI values for Vecs and clarify that certs/keys are paired by position.

-	/// Load the given certificate from disk.
+	/// Load the given certificates from disk (paired with --tls-key by position).
-	#[arg(long = "tls-cert", id = "tls-cert", env = "MOQ_SERVER_TLS_CERT")]
+	#[arg(long = "tls-cert", id = "tls-cert", env = "MOQ_SERVER_TLS_CERT", value_delimiter = ',')]
@@
-	/// Load the given key from disk.
+	/// Load the given keys from disk.
-	#[arg(long = "tls-key", id = "tls-key", env = "MOQ_SERVER_TLS_KEY")]
+	#[arg(long = "tls-key", id = "tls-key", env = "MOQ_SERVER_TLS_KEY", value_delimiter = ',')]

Also applies to: 50-55

---

228-252: Validate that the private key matches the certificate

Currently loads without verifying key/cert pairing; a mismatch will fail at handshake time. Consider checking SPKI equality before pushing CertifiedKey.

rs/moq/src/error.rs (1)

64-85: Reduce boilerplate: add a helper constructor for transport errors.

This avoids repeating map_err(|e| Error::Transport(Arc::new(e))) at call sites.

 impl Error {
   /// An integer code that is sent over the wire.
   pub fn to_code(&self) -> u32 {
@@
     }
   }
+
+  /// Helper to wrap transport-layer errors uniformly.
+  pub fn transport<E>(e: E) -> Self
+  where
+    E: std::error::Error + Send + Sync + 'static,
+  {
+    Self::Transport(Arc::new(e))
+  }
 }

rs/moq/src/session/stream.rs (1)

11-20: Open path is correct; consider using the helper for error wrapping.

Once Error::transport exists, you can shorten the mapping.

- let (send, recv) = session.open_bi().await.map_err(|err| Error::Transport(Arc::new(err)))?;
+ let (send, recv) = session.open_bi().await.map_err(Error::transport)?;

rs/moq/src/session/reader.rs (2)

7-10: Make Reader crate-internal to match Writer’s visibility.

Writer is pub(super) while Reader is pub. Unless external use is intended, align visibility to avoid semver surface expansion.

-pub struct Reader<S: web_transport_trait::RecvStream> {
+pub(super) struct Reader<S: web_transport_trait::RecvStream> {

---

20-44: Guard against unbounded buffer growth (optional).

If peers send oversized frames or malformed data, BytesMut could grow without limit. Consider a max buffer cap or Decode-level size checks to fail fast.

rs/moq/src/session/writer.rs (1)

65-70: Avoid resetting after a clean finish.

Resetting on Drop even after finish() can mis-signal errors. Track completion and only reset if not finished.

 pub(super) struct Writer<S: web_transport_trait::SendStream> {
   stream: S,
   buffer: bytes::BytesMut,
+  finished: bool,
 }
@@
   pub fn new(stream: S) -> Self {
     Self {
       stream,
       buffer: Default::default(),
+      finished: false,
     }
   }
@@
   pub async fn finish(&mut self) -> Result<(), Error> {
     self.stream.finish().await.map_err(|e| Error::Transport(Arc::new(e)))?;
+    self.finished = true;
     Ok(())
   }
@@
 impl<S: web_transport_trait::SendStream> Drop for Writer<S> {
   fn drop(&mut self) {
     // Unlike the Quinn default, we abort the stream on drop.
-    self.stream.reset(Error::Cancel.to_code());
+    if !self.finished {
+      self.stream.reset(Error::Cancel.to_code());
+    }
   }
 }

Also applies to: 49-53, 5-10, 11-17

rs/moq/src/session/publisher.rs (2)

202-276: Surface group-task errors for debugging (optional).

Currently, completions of old_group/new_group futures ignore their Result. Log errors to aid diagnosis without changing behavior.

-        Some(_) = async { Some(old_group.as_mut()?.await) } => {
+        Some(res) = async { Some(old_group.as_mut()?.await) } => {
+          if let Err(err) = res {
+            tracing::debug!(subscribe = %subscribe.id, track = %track.info.name, %err, "old group task ended with error");
+          }
           old_group = None;
           old_sequence = None;
           continue;
         },
-        Some(_) = async { Some(new_group.as_mut()?.await) } => {
+        Some(res) = async { Some(new_group.as_mut()?.await) } => {
+          if let Err(err) = res {
+            tracing::debug!(subscribe = %subscribe.id, track = %track.info.name, %err, "new group task ended with error");
+          }
           new_group = old_group;
           new_sequence = old_sequence;
           old_group = None;
           old_sequence = None;
           continue;
         },

---

263-264: Prefer debug_assert! to avoid panicking in release.

Keep the check in debug builds while preventing a hard crash in production.

-      assert!(old_group.is_none());
+      debug_assert!(old_group.is_none());

js/hang/src/frame.ts (2)

276-283: Make endianness explicit in 2‑byte varint read.

DataView defaults to big‑endian, but being explicit avoids ambiguity and future regressions.

-    v = view.getUint16(0) & 0x3fff;
+    v = view.getUint16(0, false) & 0x3fff;

---

17-29: Minor: simplify allocation math.

source and Source both expose byteLength; the ternary duplicates the same value.

-  const data = new Uint8Array(8 + (source instanceof Uint8Array ? source.byteLength : source.byteLength));
+  const data = new Uint8Array(8 + source.byteLength);
...
-  return data.subarray(0, (source instanceof Uint8Array ? source.byteLength : source.byteLength) + size);
+  return data.subarray(0, source.byteLength + size);

js/justfile (4)

11-11: Prefer explicit workspace flag for installs.

Use pnpm -w install (or pnpm -w i) to make it explicit that the install is at the workspace root.

-	pnpm i
+	pnpm -w i

---

15-20: Deterministic CI installs and clearer intent.

Minor clarity: use -w for workspace-root installs; optionally add --prefer-offline to leverage CI cache.

-check:
-	pnpm install --frozen-lockfile
+check:
+	pnpm -w install --frozen-lockfile
 	pnpm -r run check
 	pnpm -r test
 	pnpm exec biome check

---

23-26: Keep fix fast but consistent.

Use workspace flag for install; leave lockfile unfrozen for fixes.

-fix:
-	pnpm install
+fix:
+	pnpm -w install
 	pnpm exec biome check --fix

---

34-37: Align build with CI determinism.

Same note: workspace flag improves intent/readability.

-build:
-	pnpm install --frozen-lockfile
+build:
+	pnpm -w install --frozen-lockfile
 	pnpm -r run build

js/moq/src/connection.ts (3)

1-1: Consider dynamic import for the WS polyfill to trim bundle size.

Static import ships the fallback to all users. Lazy-load only if/when the fallback path is taken.

Example approach:

// lazy: only load when needed
async function connectWebSocket(url: URL, cancel: Promise<void>): Promise<WebTransport | undefined> {
	const { default: WebTransportWs } = await import("@kixelated/web-transport-ws");
	const quic = new WebTransportWs(url);
	// ... rest unchanged
}

---

46-53: Nit: feature-detect via typeof for consistency.

Minor consistency/readability tweak.

-	const webtransport = globalThis.WebTransport ? connectWebTransport(url, cancel, props?.webtransport) : undefined;
+	const webtransport =
+		typeof globalThis.WebTransport !== "undefined"
+			? connectWebTransport(url, cancel, props?.webtransport)
+			: undefined;

---

52-59: Tiny cleanup: delay promise type.

Avoid declaring a generic you don’t resolve; keeps types exact.

-			? new Promise<WebTransport>((resolve) => setTimeout(resolve, headstart)).then(() => {
+			? new Promise<void>((resolve) => setTimeout(resolve, headstart)).then(() => {

js/hang/src/connection.ts (1)

88-96: Optional: add jitter to backoff to avoid synchronized reconnects.

A small randomization reduces thundering herd when many clients reconnect simultaneously.

- this.#delay = Math.min(this.#delay * 2, this.maxDelay) as Time.Milli;
+ const jitter = 0.8 + Math.random() * 0.4; // 0.8x–1.2x
+ this.#delay = Math.min(this.#delay * 2 * jitter, this.maxDelay) as Time.Milli;

js/hang/src/watch/audio/index.ts (3)

18-18: Nit: probable typo — "JITTER_UNDERHEAD" → "JITTER_OVERHEAD".

Purely cosmetic; makes the intent clearer.

-const JITTER_UNDERHEAD = 25 as Time.Milli;
+const JITTER_OVERHEAD = 25 as Time.Milli;

And update its single usage accordingly.

---

157-166: Harden init: handle polyfill/configure failures and ensure decoder flushes on teardown.

Avoid unhandled rejections if the polyfill fails or configure throws, and flush buffered frames on close.

-    effect.spawn(async (cancel) => {
-      const loaded = await Promise.race([libav.polyfill(), cancel]);
-      if (!loaded) return; // cancelled
-
-      const decoder = new AudioDecoder({
+    effect.spawn(async (cancel) => {
+      let decoder: AudioDecoder | undefined;
+      try {
+        const loaded = await Promise.race([libav.polyfill(), cancel]);
+        if (!loaded) return; // cancelled
+
+        decoder = new AudioDecoder({
           output: (data) => this.#emit(data),
           error: (error) => console.error(error),
         });
-      effect.cleanup(() => decoder.close());
+        effect.cleanup(() => decoder?.close());
 
-      const config = info.config;
-      const description = config.description ? Hex.toBytes(config.description) : undefined;
+        const config = info.config;
+        const description = config.description ? Hex.toBytes(config.description) : undefined;
 
-      decoder.configure({
-        ...config,
-        description,
-      });
+        // Optional: preflight support check to handle "codec present but unsupported" runtimes.
+        // const { supported } = await AudioDecoder.isConfigSupported({ ...config, description });
+        // if (!supported) throw new Error("Opus decode not supported");
+
+        decoder.configure({
+          ...config,
+          description,
+        });
+      } catch (e) {
+        console.error("Audio decoder initialization failed:", e);
+        return;
+      }
+      effect.cleanup(() => decoder?.flush?.().catch(() => {}));

Also applies to: 170-174

---

109-129: Optional: race worklet module load with cancellation.

If the component is closed quickly, we can skip loading the module.

-    effect.spawn(async () => {
+    effect.spawn(async (cancel) => {
       // Register the AudioWorklet processor
-      await context.audioWorklet.addModule(RenderWorklet);
+      const loaded = await Promise.race([context.audioWorklet.addModule(RenderWorklet), cancel]);
+      if (!loaded) return; // cancelled

js/hang/src/util/libav.ts (2)

5-7: Optional: presence check can be too coarse.

Browsers may expose AudioDecoder but lack Opus support. Consider a codec-specific preflight (e.g., Opus) and only load the polyfill if unsupported.

Example:

// Call site (watch/publish) before configure:
const { supported } = await AudioDecoder.isConfigSupported({
  codec: "opus",
  numberOfChannels,
  sampleRate,
  description,
});
if (!supported) await libav.polyfill();

---

9-26: Replace require with dynamic import() and add error handling
In js/hang/src/util/libav.ts, use dynamic imports for @libav.js/variant-opus and libavjs-webcodecs-polyfill wrapped in a try/catch to log failures and return false, improving ESM and SSR compatibility.

     // NOTE: we use require here to avoid tsc errors with libavjs-webcodecs-polyfill.
-    loading = Promise.all([require("@libav.js/variant-opus"), require("libavjs-webcodecs-polyfill")]).then(
-      async ([opus, libav]) => {
-        await libav.load({
-          LibAV: opus,
-          polyfill: true,
-        });
-        return true;
-      },
-    );
+    loading = (async () => {
+      try {
+        const [opus, libav]: any = await Promise.all([
+          import("@libav.js/variant-opus"),
+          import("libavjs-webcodecs-polyfill"),
+        ]);
+        await libav.load({ LibAV: opus, polyfill: true });
+        return true;
+      } catch (err) {
+        console.error("Failed to load Opus polyfill:", err);
+        return false;
+      }
+    })();

[js/hang/src/util/libav.ts:15]

js/hang/src/publish/audio/index.ts (3)

228-236: Add lightweight backpressure guard before allocating/copying.

Avoids latency blowups when encodeQueue backs up.

-      worklet.port.onmessage = ({ data }: { data: Capture.AudioFrame }) => {
+      worklet.port.onmessage = ({ data }: { data: Capture.AudioFrame }) => {
+        if (encoder.encodeQueueSize > 2) {
+          // Drop under pressure to keep latency bounded.
+          return;
+        }
         const channels = data.channels.slice(0, settings.channelCount);
         const joinedLength = channels.reduce((a, b) => a + b.length, 0);
         const joined = new Float32Array(joinedLength);

Optional: cache and reuse a Float32Array when joinedLength is stable to reduce GC pressure.

---

193-197: Optional: surface polyfill readiness for observability.

Consider logging/metric when the polyfill path is taken vs native, to aid debugging on Safari.

---

228-236: Minor: assume equal-channel lengths to avoid first reduce.

If capture guarantees equal lengths per channel, you can compute joinedLength as channels.length * channels[0].length and skip one pass. Leave as-is if not guaranteed.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

Learnt from: CR
PR: kixelated/moq#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-13T21:30:35.001Z
Learning: Applies to justfile : Use just as the task runner; define developer tasks in the repository justfile

Learnt from: CR
PR: kixelated/moq#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-13T21:30:35.001Z
Learning: Applies to rs/Cargo.toml : Maintain the Rust workspace configuration in rs/Cargo.toml